Add ability to create test users with magic link logins - #2062

@jbouquiaux could you test and review (probably more the backend part) this ? Likely we can add something in the dev-docker documentation with the command to add a user with the docker compose (something like "docker compose exec api-dev uv run invoke dev.create-test-user --username gloubs --role user")
@DianaAliabieva could you test and review (probably more the frontend part) this ?

Codecov Report

❌ Patch coverage is 70.00000% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.57%. Comparing base (53c6afb) to head (a6d9655).

@@            Coverage Diff             @@
##             main    #2062      +/-   ##
==========================================
- Coverage   81.59%   81.57%   -0.02%     
==========================================
  Files          88       88              
  Lines        8481     8489       +8     
==========================================
+ Hits         6920     6925       +5     
- Misses       1561     1564       +3     

Hi @davidwaroquiers, thanks for this. We definitely need something that solves this problem but I think I've come down against the idea of adding password-based login in this way.

I think my suggestion would be instead to hook into the email login approach and enable that by default on dev servers. We can then make invoke dev.serve provide an authenticated link with a token for testing the developers "normal" user by default, and an extra invoke task very similar to the one you added here that creates a test-user and prints a token link for the developer.

What do you think? Happy to implement this if you are okay with it, or go back to this idea if you still prefer a password.

Hi @ml-evs

Thank you for the message. Just to mention that it is even not with a password it's "easier". Here is with a few screenshots:

1/ Login page:

2/ List of clickable passwordless users with some info (their role, the group(s) they belong to):

And you just click on the user you want to be logged in. That eases tests with different users with different roles, belonging to different groups, in the future it could also help test group admins, project admins etc ...).

It is quite perpendicular to almost everything in datalab. It only touches four small points:

• The configuration flag exposed in the /info response (ENABLE_UNSAFE_TESTING_PASSWORDLESS_LOGIN -- could be renamed as it is very long but at least it's clear :D)
• The invoke dev.create-test-user task (only active when the above flag is True)
• Two endpoints on the existing AUTH blueprint group: "GET /login/testing-passwordless/users" to get the list of test users, and "POST /login/testing-passwordless" to start the normal Flask login session for the selected (clicked) user
• The user picker with the buttons shown above
  (note that one thing I did not like is that there was a new testing_passwordless IdentityType... I removed it in the last commit, using a workaround with the email IdentityType)

After a user is selected, everything follows the normal datalab "flow path" (using the Flask login session for access etc ...).

What do you think ?

Ah sorry, I think I just searched for "password" and thought I was responding to #1898 🙃 I guess my question then becomes whether adding this to the UI itself is that useful, and if it really needs an API route? I'd prefer a task that lists these login users in the terminal and offers login links for each. Do you find the extra UI component that much more useful than having e.g., a few private browser windows logged in as different accounts?

Ah sorry, I think I just searched for "password" and thought I was responding to #1898 🙃 I guess my question then becomes whether adding this to the UI itself is that useful, and if it really needs an API route? I'd prefer a task that lists these login users in the terminal and offers login links for each. Do you find the extra UI component that much more useful than having e.g., a few private browser windows logged in as different accounts?

Not that much more useful indeed. I think the idea of having one invoke task that gives the list of passwordless users and their "properties" (roles, groups, ...) and the link is all good. I'll remove the UI part. As for the API route, we at least need the one to login no ?

Done, should be ready for review

Could you take a look at 95aa6b0 @davidwaroquiers? I think this is the same functionality but without needing any extra config or routes, just lets you trigger token creation using your test accounts and prints the links you would use to login

Could you take a look at 95aa6b0 @davidwaroquiers? I think this is the same functionality but without needing any extra config or routes, just lets you trigger token creation using your test accounts and prints the links you would use to login

Hi @ml-evs

Thanks! Works perfectly and indeed much better that there is no new route for that. My only "concern"/"question" is about the config option. Correct me if I'm wrong but we can create a test user for "any" deployment in this case. Just wondering if it wouldn't be better to keep the config option as a "gate" to prevent from creating test users in a "normal/production" deployment. When the test-user option is active (or when other security problems are there, e.g. secrets not matching or whatever), there could be a warning banner to inform the user that the deployment is not meant for production because of this or that. What do you think ? I'm fine either way if you think that is not needed (and anyway if we find out later that it would be useful, it can always be added later).

Thanks!

Thanks! Works perfectly and indeed much better that there is no new route for that.

Great, I've added it to the end of this PR.

My only "concern"/"question" is about the config option. Correct me if I'm wrong but we can create a test user for "any" deployment in this case. Just wondering if it wouldn't be better to keep the config option as a "gate" to prevent from creating test users in a "normal/production" deployment. When the test-user option is active (or when other security problems are there, e.g. secrets not matching or whatever), there could be a warning banner to inform the user that the deployment is not meant for production because of this or that. What do you think ? I'm fine either way if you think that is not needed (and anyway if we find out later that it would be useful, it can always be added later).

It's a fair point, it needs direct database access (at which point you could do a lot anyway). I've just tweaked this PR so it only creates the token links for emails on the .test TLD and embedded an extra variable in the token's JWT that says it was created in this way, and only lets you login with these links when "TESTING" is enabled. This pre-empts #2121 a little bit, where I'm changing testing to have not affect on auth at all -- this will be the one effect that remains -- letting you login with these test accounts.

It also goes back to only listing users with the .test domain (and renamed back to dev.list-test-users), rather than all users.

Thanks! Works perfectly and indeed much better that there is no new route for that.

Great, I've added it to the end of this PR.

My only "concern"/"question" is about the config option. Correct me if I'm wrong but we can create a test user for "any" deployment in this case. Just wondering if it wouldn't be better to keep the config option as a "gate" to prevent from creating test users in a "normal/production" deployment. When the test-user option is active (or when other security problems are there, e.g. secrets not matching or whatever), there could be a warning banner to inform the user that the deployment is not meant for production because of this or that. What do you think ? I'm fine either way if you think that is not needed (and anyway if we find out later that it would be useful, it can always be added later).

It's a fair point, it needs direct database access (at which point you could do a lot anyway). I've just tweaked this PR so it only creates the token links for emails on the .test TLD and embedded an extra variable in the token's JWT that says it was created in this way, and only lets you login with these links when "TESTING" is enabled. This pre-empts #2121 a little bit, where I'm changing testing to have not affect on auth at all -- this will be the one effect that remains -- letting you login with these test accounts.

It also goes back to only listing users with the .test domain (and renamed back to dev.list-test-users), rather than all users.

Perfect for me!

Do you want me to make a final review ?

Perfect for me!

Do you want me to make a final review ?

If you have time -- just trying to tidy up merge/rebase! I'll also start work on the follow up PRs

pre-commit.ci run

pre-commit.ci autofix